feat(java,info): support multi-property in yaml #780
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #780 +/- ##
============================================
+ Coverage 76.59% 76.68% +0.08%
- Complexity 560 583 +23
============================================
Files 83 84 +1
Lines 8679 8736 +57
Branches 1006 1012 +6
============================================
+ Hits 6648 6699 +51
- Misses 1813 1814 +1
- Partials 218 223 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hey @Thespica, please help me review. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for multi-valued properties (LIST and SET cardinality) in GraphAr's Java implementation, enabling properties to hold multiple values instead of just single values. The implementation introduces a new Cardinality enum and integrates it throughout the info layer for YAML serialization/deserialization.
Key Changes
- Introduces
Cardinalityenum (SINGLE, LIST, SET) to distinguish single-valued from multi-valued properties - Updates Property, PropertyYaml, and related info classes to support cardinality metadata
- Adds validation rules: edges only support SINGLE cardinality, and CSV files don't support multi-cardinality properties
- Enhances BaseGraphInfoSaver to auto-generate filenames when saving to directories
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
maven-projects/info/src/main/java/org/apache/graphar/info/type/Cardinality.java |
New enum defining SINGLE, LIST, and SET cardinality types with string conversion methods |
maven-projects/info/src/main/java/org/apache/graphar/info/Property.java |
Adds cardinality field and constructors to support multi-valued properties |
maven-projects/info/src/main/java/org/apache/graphar/info/yaml/PropertyYaml.java |
Adds cardinality field for YAML serialization, with special handling to omit SINGLE from output |
maven-projects/info/src/main/java/org/apache/graphar/info/PropertyGroup.java |
Adds getCardinality() method and validation to reject multi-cardinality in CSV files |
maven-projects/info/src/main/java/org/apache/graphar/info/VertexInfo.java |
Exposes getCardinality() method and adds validation calls before dumping |
maven-projects/info/src/main/java/org/apache/graphar/info/EdgeInfo.java |
Adds validation to reject multi-cardinality properties in edges and validation calls before dumping |
maven-projects/info/src/main/java/org/apache/graphar/info/GraphInfo.java |
Adds validation calls before dumping YAML |
maven-projects/info/src/main/java/org/apache/graphar/info/yaml/VertexYaml.java |
Adds unused labels field (unrelated to multi-property feature) |
maven-projects/info/src/main/java/org/apache/graphar/info/saver/BaseGraphInfoSaver.java |
Adds auto-filename generation when URIs are directories |
maven-projects/info/src/test/java/org/apache/graphar/info/MultiPropertyTest.java |
Comprehensive test suite for cardinality functionality including YAML round-trip and validation |
maven-projects/info/src/test/java/org/apache/graphar/info/TestDataFactory.java |
Adds factory method for creating properties with cardinality |
maven-projects/info/src/test/java/org/apache/graphar/info/TestUtil.java |
Adds helper methods to access LDBC test data with multi-property support |
maven-projects/info/src/test/java/org/apache/graphar/info/TestVerificationUtils.java |
Adds cardinality assertion to property comparison |
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java |
Adds cardinality assertions to existing property tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| isValidated(); | ||
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | ||
| VertexYaml vertexYaml = new VertexYaml(this); | ||
| yaml.dump(vertexYaml, output); | ||
| } | ||
|
|
||
| public String dump() { | ||
| isValidated(); |
There was a problem hiding this comment.
The call to isValidated() doesn't use its return value. If the intent is to validate before dumping, this should either throw an exception when validation fails, or check the return value and throw an appropriate exception. As written, invalid VertexInfo objects can still be dumped to YAML.
| isValidated(); | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| VertexYaml vertexYaml = new VertexYaml(this); | |
| yaml.dump(vertexYaml, output); | |
| } | |
| public String dump() { | |
| isValidated(); | |
| if (!isValidated()) { | |
| throw new IllegalStateException("VertexInfo object is not valid and cannot be dumped."); | |
| } | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| VertexYaml vertexYaml = new VertexYaml(this); | |
| yaml.dump(vertexYaml, output); | |
| } | |
| public String dump() { | |
| if (!isValidated()) { | |
| throw new IllegalStateException("VertexInfo object is not valid and cannot be dumped."); | |
| } |
| PropertyYaml invalidYaml = new PropertyYaml(); | ||
| invalidYaml.setName("invalid_prop"); | ||
| invalidYaml.setData_type("string"); | ||
| invalidYaml.setCardinality("INVALID"); // This should default to SINGLE |
There was a problem hiding this comment.
The comment states "This should default to SINGLE" but the test correctly expects an IllegalArgumentException to be thrown. The comment is misleading and should be updated to reflect that invalid cardinality values should throw an exception rather than defaulting to SINGLE.
| invalidYaml.setCardinality("INVALID"); // This should default to SINGLE | |
| invalidYaml.setCardinality("INVALID"); // This should throw an IllegalArgumentException |
| isValidated(); | ||
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | ||
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | ||
| yaml.dump(graphYaml, output); | ||
| } | ||
|
|
||
| public String dump(URI storeUri) { | ||
| isValidated(); | ||
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | ||
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | ||
| return yaml.dump(graphYaml); | ||
| } | ||
|
|
||
| public String dump() { | ||
| isValidated(); |
There was a problem hiding this comment.
The call to isValidated() doesn't use its return value. If the intent is to validate before dumping, this should either throw an exception when validation fails, or check the return value and throw an appropriate exception. As written, invalid GraphInfo objects can still be dumped to YAML.
| isValidated(); | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| yaml.dump(graphYaml, output); | |
| } | |
| public String dump(URI storeUri) { | |
| isValidated(); | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| return yaml.dump(graphYaml); | |
| } | |
| public String dump() { | |
| isValidated(); | |
| if (!isValidated()) { | |
| throw new IllegalStateException("GraphInfo is not valid and cannot be dumped."); | |
| } | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| yaml.dump(graphYaml, output); | |
| } | |
| public String dump(URI storeUri) { | |
| if (!isValidated()) { | |
| throw new IllegalStateException("GraphInfo is not valid and cannot be dumped."); | |
| } | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| return yaml.dump(graphYaml); | |
| } | |
| public String dump() { | |
| if (!isValidated()) { | |
| throw new IllegalStateException("GraphInfo is not valid and cannot be dumped."); | |
| } |
| isValidated(); | ||
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | ||
| EdgeYaml edgeYaml = new EdgeYaml(this); | ||
| yaml.dump(edgeYaml, output); | ||
| } | ||
|
|
||
| public String dump() { | ||
| isValidated(); |
There was a problem hiding this comment.
The call to isValidated() doesn't use its return value. If the intent is to validate before dumping, this should either throw an exception when validation fails, or check the return value and throw an appropriate exception. As written, invalid EdgeInfo objects can still be dumped to YAML.
| isValidated(); | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| EdgeYaml edgeYaml = new EdgeYaml(this); | |
| yaml.dump(edgeYaml, output); | |
| } | |
| public String dump() { | |
| isValidated(); | |
| if (!isValidated()) { | |
| throw new IllegalStateException("EdgeInfo object is not valid and cannot be dumped."); | |
| } | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| EdgeYaml edgeYaml = new EdgeYaml(this); | |
| yaml.dump(edgeYaml, output); | |
| } | |
| public String dump() { | |
| if (!isValidated()) { | |
| throw new IllegalStateException("EdgeInfo object is not valid and cannot be dumped."); | |
| } |
| this.data_type = ""; | ||
| this.is_primary = false; | ||
| this.is_nullable = Optional.empty(); | ||
| this.cardinality = "single"; // Default to single |
There was a problem hiding this comment.
Setting a default value of "single" for the cardinality field in the no-arg constructor is problematic. When YAML deserialization occurs and no cardinality field is present, the field will remain as the default "single". However, the getCardinality() method (line 89) returns null for "single" cardinality to avoid serializing it. This creates an inconsistency. Consider initializing cardinality to null instead, and handle the default in the Property constructor that takes PropertyYaml.
| this.cardinality = "single"; // Default to single | |
| this.cardinality = null; // No default; handle in Property constructor |
| isValidated(); | ||
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | ||
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | ||
| yaml.dump(graphYaml, output); | ||
| } | ||
|
|
||
| public String dump(URI storeUri) { | ||
| isValidated(); | ||
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | ||
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | ||
| return yaml.dump(graphYaml); | ||
| } | ||
|
|
||
| public String dump() { | ||
| isValidated(); |
There was a problem hiding this comment.
The call to isValidated() doesn't use its return value. If the intent is to validate before dumping, this should either throw an exception when validation fails, or check the return value and throw an appropriate exception. As written, invalid GraphInfo objects can still be dumped to YAML.
| isValidated(); | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| yaml.dump(graphYaml, output); | |
| } | |
| public String dump(URI storeUri) { | |
| isValidated(); | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| return yaml.dump(graphYaml); | |
| } | |
| public String dump() { | |
| isValidated(); | |
| if (!isValidated()) { | |
| throw new IllegalStateException("GraphInfo is not valid and cannot be dumped."); | |
| } | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| yaml.dump(graphYaml, output); | |
| } | |
| public String dump(URI storeUri) { | |
| if (!isValidated()) { | |
| throw new IllegalStateException("GraphInfo is not valid and cannot be dumped."); | |
| } | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| return yaml.dump(graphYaml); | |
| } | |
| public String dump() { | |
| if (!isValidated()) { | |
| throw new IllegalStateException("GraphInfo is not valid and cannot be dumped."); | |
| } |
| isValidated(); | ||
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | ||
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | ||
| yaml.dump(graphYaml, output); | ||
| } | ||
|
|
||
| public String dump(URI storeUri) { | ||
| isValidated(); | ||
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | ||
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | ||
| return yaml.dump(graphYaml); | ||
| } | ||
|
|
||
| public String dump() { | ||
| isValidated(); |
There was a problem hiding this comment.
The call to isValidated() doesn't use its return value. If the intent is to validate before dumping, this should either throw an exception when validation fails, or check the return value and throw an appropriate exception. As written, invalid GraphInfo objects can still be dumped to YAML.
| isValidated(); | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| yaml.dump(graphYaml, output); | |
| } | |
| public String dump(URI storeUri) { | |
| isValidated(); | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| return yaml.dump(graphYaml); | |
| } | |
| public String dump() { | |
| isValidated(); | |
| if (!isValidated()) { | |
| throw new IllegalStateException("GraphInfo is not valid and cannot be dumped."); | |
| } | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| yaml.dump(graphYaml, output); | |
| } | |
| public String dump(URI storeUri) { | |
| if (!isValidated()) { | |
| throw new IllegalStateException("GraphInfo is not valid and cannot be dumped."); | |
| } | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| GraphYaml graphYaml = new GraphYaml(storeUri, this); | |
| return yaml.dump(graphYaml); | |
| } | |
| public String dump() { | |
| if (!isValidated()) { | |
| throw new IllegalStateException("GraphInfo is not valid and cannot be dumped."); | |
| } |
| if (p.getCardinality() != Cardinality.SINGLE) { | ||
| // edge property only supports single cardinality | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[nitpick] EdgeInfo doesn't expose a getCardinality(String propertyName) method like VertexInfo does (line 89 in VertexInfo.java). For API consistency, consider adding this method to EdgeInfo as well, which would always return Cardinality.SINGLE for valid edge properties. This would provide a uniform API across both VertexInfo and EdgeInfo.
| isValidated(); | ||
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | ||
| EdgeYaml edgeYaml = new EdgeYaml(this); | ||
| yaml.dump(edgeYaml, output); | ||
| } | ||
|
|
||
| public String dump() { | ||
| isValidated(); |
There was a problem hiding this comment.
The call to isValidated() doesn't use its return value. If the intent is to validate before dumping, this should either throw an exception when validation fails, or check the return value and throw an appropriate exception. As written, invalid EdgeInfo objects can still be dumped to YAML.
| isValidated(); | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| EdgeYaml edgeYaml = new EdgeYaml(this); | |
| yaml.dump(edgeYaml, output); | |
| } | |
| public String dump() { | |
| isValidated(); | |
| if (!isValidated()) { | |
| throw new IllegalStateException("EdgeInfo object is not valid and cannot be dumped."); | |
| } | |
| Yaml yaml = new Yaml(GraphYaml.getRepresenter(), GraphYaml.getDumperOptions()); | |
| EdgeYaml edgeYaml = new EdgeYaml(this); | |
| yaml.dump(edgeYaml, output); | |
| } | |
| public String dump() { | |
| if (!isValidated()) { | |
| throw new IllegalStateException("EdgeInfo object is not valid and cannot be dumped."); | |
| } |
| LIST, | ||
| /** Set of values property (no duplicates) */ | ||
| SET; | ||
|
|
There was a problem hiding this comment.
This method overrides Enum.toString; it is advisable to add an Override annotation.
| @Override |
2 participants
Reason for this PR
close #778
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?